wizard: add Trezor Safe 7 support - #4674
Conversation
The Trezor Safe 7 speaks the Trezor Host Protocol v2 rather than the legacy v1 wire protocol. With THP implemented in the wallet, the GUI needs three things to drive it. A pairing dialog for the THP CodeEntry step: on the first pairing the device displays a six-digit code and the host has to echo it back. The dialog collects it, accepts the spacing and dashes a paste brings with it, and reports a rejected code in place so the user can try again instead of dead-ending the wizard. The Safe 7 as a selectable model in the create-wallet-from-device wizard, with its own image. Classification of Trezor connect and open failures, so the loading screen can offer a retry hint when the device is merely off, locked or not yet paired, rather than the "check application logs" error that every failure produces today. Cancelling, on the device or in the dialog, now ends the wizard quietly instead of as an error, which is what the second argument to walletCreatedFromDevice carries. C++17 is set unconditionally rather than only for DEV_MODE checkouts, because the monero revision carrying THP requires it. That hunk stands on its own and can be dropped if it would rather go in with the submodule bump. No submodule bump is included here. The matching monero change is a separate pull request, and moving the pointer before that merges would pin the GUI to a personal fork.
5e96d1e to
9e26854
Compare
|
Heads up for anyone reading this in advance, I've been chipping away at implementing Trezor Safe 7 support for the last couple months and I think I have an implementation that is pretty solid, or as solid as someone who is not familiar with Monero development could be. Massive heads up that this PR and all code in it was developed via Claude Code and although I've spent a lot of time iterating and researching to do my best to ensure compliance with the Monero development practices and policies, as well as the Trezor practices and policies, I do not have a complete and in depth understanding of all the changes, so I want to make sure to call this out for the sake of transparency. This message is from myself and not generated, and for any feedback or review, you'll be speaking with me directly. I have tested this extensively on Linux so far with a couple of my personal devices, but I haven't tested on any other platforms yet. I'm planning to test on at least windows, and potentially some other platforms such as Android, but I won't have access to an Apple developer account for testing on something like MacOS. I'll be doing a bit more testing on linux and doing some windows testing as well before moving this out of draft, which I plan to do later this week. I'm happy to work to address feedback that comes up but some of those requests may require resources or time that I don't have. So in that case, these PR's will serve as a potential base for more common Monero contributors to work off of, or as a potential unblocker for some people in the community who want Trezor Safe 7 support now, before the core team finishes their implementation. |
|
I think a PR that integrates Trezor's Rust implementation would have been preferable. Thousands of lines of AI written code implementing Trezor's protocol on C++ is not maintainable unfortunately. |
I can change the implementation for both PR's to Rust. I'm leaving for my own work at the moment. I'll get back to this either tomorrow or some other time this week. Please keep in mind, these are draft PR's for a reason. I was never planning to submit this work directly, as my comment mentions. Typically when I submit draft PR's I don't move them out of draft after they're ready, I typically close and re-open, to clean things up for full review. So this was always going to be closed in the end. The largest reason that I opened up these draft PRs now rather than after I finished polishing everything is because I want to unblock people who want a solution and support for their devices now since it's been quite some time since the device came out and there's lots of talk in the community about wanting this. These PRs will be visible to others searching for info about support. And they can use it now if they'd like. |
|
Actually, would it be possible to use |
3 participants
What this does
This is the GUI half of Trezor Safe 7 support. The protocol work is in
monero-project/monero#11043; this PR adds the user-facing part of it.The Safe 7 requires the Trezor Host Protocol v2, which pairs by showing a six-digit code on the
device screen that the user types into the host. There is nothing in the GUI today that can ask
for that, so this adds it.
Draft, and what it is waiting on
Marked draft deliberately. It should not merge before the core PR does, because the pairing
callback it binds to does not exist until then.
This PR does not bump the
monerosubmodule, and that is intentional. monero-gui pins therelease-v0.18line, and the pointer is release-prep territory rather than something a feature PRtouches: of the 51 commits that have ever moved it, every one since
9c76f099back torepo: prepare v0.17.1.1in October 2020 is a release-prep commit. The last bump that was not ismonero: update submodule to master branch HEAD, 2020-08-28. Moving it here would also mean a~2178-commit jump across a branch divergence, which is not something to smuggle into a feature
change.
So expect CI on this PR to be red, and expect it to stay red for a while. I would rather say
that up front than have it look like a broken branch:
build-macos,build-ubuntuandbuild-windowsrunDEV_MODE=ON make release, which buildsagainst monero
origin/master. They are red until the core PR merges to master, and they gogreen on their own at that point, with no submodule bump at all.
macos-bundle,docker-linux-static,docker-windows-staticanddocker-androidbuild againstthe pinned pointer, so they stay red until THP reaches the pinned line. That is a release-cycle
question rather than something this PR should try to answer.
I would rather raise that timing question with you than guess at it. If the preference is for this
to wait for a
release-v0.18backport, or to ride a normal bump cycle, that is fine by me — I justneed to know which, so I can keep the branch in the right shape.
What the user sees
the GUI asks for it. On success the device issues a credential that is stored, so this happens
once per device rather than once per connection.
dead-ending.
"try again" from "this will not work" — for example a device that is not connected versus firmware
that does not support Monero.
Existing hardware wallets are unaffected. The pairing dialog only appears for a device that asks
for pairing.
One thing I would rather ask about than assume:
images/trezor7.pngsits alongside the existingtrezor3.png,trezor5.pngandtrezorT.pngand is used the same way. If the project has aprovenance or licensing requirement for device artwork that I have not met, say so and I will
replace it, or drop the image and let the Safe 7 fall back to the generic entry.
How the dialog talks to the device thread
It follows the existing
PassphraseHelperpattern rather than inventing a new mechanism: thedevice thread blocks on a condition variable while the QML dialog collects input on the UI thread,
with no
BlockingQueuedConnectionand no lock held across the UI round trip.Not part of THP
It is one commit, so I want to point at the hunks in it that are not Safe 7 work rather than let
them hide:
CMakeLists.txtsetsCMAKE_CXX_STANDARD 17unconditionally, where today it is 17 only forDEV_MODEcheckouts and 14 otherwise. The monero revision that carries THP requires C++17. Thishunk is independent of the rest and could equally well ride along with a future submodule bump.
Ledger.
the app.
Say the word on any of the three and I will pull it out entirely.
Building and reviewing this
This needs the core branch checked out in the submodule, since the pairing callback it binds to
does not exist on master yet:
Do not use
make devmode— withDEV_MODE=ON, CMake force-checks-outorigin/masterin thesubmodule and will destroy your checkout. Without
MANUAL_SUBMODULES=1and withDEV_MODE=OFF,CMake hard-errors on the submodule mismatch instead.
Testing
Automated. The existing QML suite still passes with these changes, which matters here because
this branch touches
WizardController.qmlandWizardCreateDevice1.qml, andtst_Wizard.qmlcovers exactly that path. Run the same way CI does —
xvfb-run -a build/bin/monero-wallet-gui --test-qml— on Qt 5.15.17, Release, built against the core branch:I have not added QML tests for the pairing dialog itself. It needs a device callback to drive it,
and faking one convincingly enough to be worth the harness is more machinery than the test would
justify. If you would rather have it, I will add it.
Hardware: not tested yet, which along with the core dependency is why this is a draft. I will
post as a comment: device and firmware version, first pairing, reconnect with a stored credential,
a wrong code, cancelling at each step, a signed transaction, and a legacy Model T / Safe 3 / Safe 5
still working. The pairing dialog in particular has failure paths that only a real device
exercises, so I do not want the QML result above read as more than it is.
Qt versions. Built and tested against Qt 5.15.17 only. The README states a 5.12 floor and
CI uses 5.15, so a 5.15-only QML construct is exactly the kind of thing neither of us would catch,
and I do not want to imply coverage I do not have.
What I can say is narrower and checked rather than assumed:
DevicePairingCodeDialog.qmldeclaresno
Connectionsblock at all, so it cannot be relying on the 5.15function onSignalName()handler form, and it imports nothing above the versions the rest of the file tree already imports.
If someone with a 5.12 environment can run it, I would rather have that than my reasoning. Worth
knowing either way:
components/DatePicker.qmlalready uses a 5.15-onlyConnectionsform onmaster, so a 5.12 failure there is pre-existing and not from this branch.
How I would like to work on this
The same as on the core PR, and for the same reason: this is a free contribution, Safe 7 support
had not been started by anyone, and I would rather it existed than that this particular diff did.
Take it and change it — you do not need me in the loop to restructure or rewrite any of it. I will
answer review comments here and keep the branch rebased. What I cannot do is an open-ended series
of large reworks that need hardware or platforms I do not have; I have one Safe 7 and one Linux
machine, no macOS or Windows build host, and no way to test the Qt 5.12 floor. I will not be at
meetings — everything I have to say is here, and I will act on any decision made elsewhere if
someone points me at it.